Skip to content

Introduction of timing attack safe bcmp implementation. - #4729

Closed
devnexen wants to merge 2 commits into
php:masterfrom
devnexen:safe_bcmp_intro
Closed

Introduction of timing attack safe bcmp implementation.#4729
devnexen wants to merge 2 commits into
php:masterfrom
devnexen:safe_bcmp_intro

Conversation

@devnexen

Copy link
Copy Markdown
Member

Nothing new but to refactor usage b/w hash and password
extensions but using volatile pointers to be a bit safer,
allowing to expand its usage eventually.

Comment thread main/safe_bcmp.c Outdated
@devnexen

devnexen commented Jun 6, 2020

Copy link
Copy Markdown
Member Author

ping :) @nikic @staabm

@cmb69

cmb69 commented Dec 28, 2021

Copy link
Copy Markdown
Member

Would it make sense to add the length check (both strings must have same length) to php_safe_bcmp() (and accepting zend_strings instead of void*s)? Otherwise, I'd like to see a respective comment.

Anyway, could you please resolve the merge conflicts?

@devnexen

devnexen commented Dec 28, 2021

Copy link
Copy Markdown
Member Author

Sure I can give it a try

@devnexen
devnexen force-pushed the safe_bcmp_intro branch 2 times, most recently from 757af67 to 955eaee Compare December 28, 2021 17:05
@cmb69

cmb69 commented Dec 29, 2021

Copy link
Copy Markdown
Member

This looks reasonable to me (not sure about adding a new file for that). More review welcome!

@ramsey

ramsey commented May 31, 2022

Copy link
Copy Markdown
Member

What's the status of this PR? Still waiting for review? Can we get this into master for 8.2?

@ramsey ramsey added this to the PHP 8.2 milestone May 31, 2022
@devnexen

devnexen commented Jun 9, 2022

Copy link
Copy Markdown
Member Author

ping :-)

@Girgias Girgias left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM other than the nits

Comment thread main/safe_bcmp.c Outdated
Comment thread main/php.h Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add a comment either here to indicate that -1 is if the strings do not have the same lengths, 0 if they match and 1 if they are not equal?

devnexen added 2 commits June 20, 2022 13:10
Nothing new but to refactor usage b/w hash and password
extensions but using volatile pointers to be a bit safer,
allowing to expand its usage eventually.
@devnexen

Copy link
Copy Markdown
Member Author

Closed by bfe6f9e66

@devnexen devnexen closed this Jun 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants